Use named loggers instead of setting root logger in natlinkcore library #72

LexiconCode · 2024-03-30T08:25:51Z

This pr use named loggers instead of setting root logger in natlinkcore library. Fixing #71

Changes to loader.py

Lines 669 to 699 in 2dd7df4

    
           def run() -> None: 
        
               default_logger=logging.getLogger() 
        
               dh = OutputDebugStringHandler() 
        
               sh=logging.StreamHandler(sys.stdout) 
        
               for h in [sh,dh]: 
        
                   default_logger.addHandler(h) 
        
               default_logger.setLevel(logging.DEBUG) 
        
               logging.debug(f"{__file__} run()") 
        
               try: 
        
                   # # TODO: remove this hack. As of October 2021, win32api does not load properly, except if 
        
                   # # the package pywin32_system32 is explictly put on new dll_directory white-list 
        
                   # pywin32_dir = os.path.join(sysconfig.get_path('platlib'), "pywin32_system32") 
        
                   # if os.path.isdir(pywin32_dir): 
        
                   #     os.add_dll_directory(pywin32_dir) 
        
                   #create a temporary logging handler, so we can log the startup of DAP 
        
                   config = NatlinkConfig.from_first_found_file(config_locations()) 
        
                   dap_started = config.dap_enabled and startDap(config)            
        
                   logger=logging.getLogger("natlink") 
        
                   logger.setLevel(logging.DEBUG) 
        
                   main = NatlinkMain(logger, config) 
        
                   main.setup_logger() 
        
                   main.dap_started=dap_started 
        
                   for h in [sh,dh]: 
        
                       default_logger.removeHandler(h)

'OutputDebugStringHandler' is from natlink there for logger is named natlink instead setting the root logger. If natlinkcore main fails to loads this logger should catch it.
Rename natlink to natlinkcore as the is loading natlinkcore after NatlinkConfig.

natlinkcore/src/natlinkcore/loader.py

Line 691 in 2dd7df4

logger=logging.getLogger("natlink")

LexiconCode · 2024-03-30T08:28:15Z

Test setting debug in config:

set log level to: DEBUG
Starting natlink loader from config file:
	"C:\Users\MainLaptop\.natlink\natlink.ini"
get_user_language, return userLanguage: "enx", (long language: "US English")
set_user_language, user: "MainLaptop", profile: "C:\ProgramData\Nuance\NaturallySpeaking13\Users\MainLaptop\current", language: "enx"
triggering load/reload process
loading module: _dragonfly_example_rule
WARNING:monitor.init:DPI awareness could not be set; it has been set already.
on_change_callback called with: "mic", "on"
triggering load/reload process
-on_begin_callback, new module info: ( (...)natspeak, Messages from Natlink, 3344720 )
triggering load/reload process
-on_begin_callback, same moduleInfo
on_change_callback called with: "mic", "off"

WARNING:monitor.init:DPI awareness could not be set; it has been set already. is from dragonfly.

LexiconCode · 2024-03-30T08:33:37Z

A few questions before merging.

Should we be using NullHandler instead of Named logger for Natlink?

From: docs.python.org/3/howto/logging.html#configuring-logging-for-a-library
"Note
It is strongly advised that you do not log to the root logger in your library. Instead, use a logger with a unique and easily identifiable name, such as the name for your library’s top-level package or module. Logging to the root logger will make it difficult or impossible for the application developer to configure the logging verbosity or handlers of your library as they wish.

Note
It is strongly advised that you do not add any handlers other than NullHandler to your library’s loggers. This is because the configuration of handlers is the prerogative of the application developer who uses your library. The application developer knows their target audience and what handlers are most appropriate for their application: if you add handlers ‘under the hood’, you might well interfere with their ability to carry out unit tests and deliver logs which suit their requirements."

Could the logging could be simplified under one handler?

drmfinlay · 2024-03-30T10:27:25Z

I can answer these questions.

Should we be using NullHandler instead of Named logger for Natlink?

Loggers and logging handlers are different things. Natlink should be using a named logger as you have done in this PR. Natlink should not be using a NullHandler because it is not a library. Its log output should appear in the messages window. Hence, using StreamHandler is correct.

Could the logging could be simplified under one handler?

I don't think so. It looks like both logging handlers are important. One writes to stdout and one is used for remote debugging.

LexiconCode added the bug Something isn't working label Mar 30, 2024

LexiconCode requested a review from dougransom March 30, 2024 08:25

LexiconCode marked this pull request as draft March 30, 2024 08:34

LexiconCode changed the title ~~Use named loggers instead of setting root logger in natlinkcore library.~~ Use named loggers instead of setting root logger in natlinkcore library? Mar 30, 2024

Use named logger instead of setting root logger in natlinkcore library.

09108af

LexiconCode force-pushed the LexiconCode-log-patch branch from e08548b to 09108af Compare March 30, 2024 09:32

LexiconCode changed the title ~~Use named loggers instead of setting root logger in natlinkcore library?~~ Use named loggers instead of setting root logger in natlinkcore library Mar 30, 2024

LexiconCode self-assigned this Mar 30, 2024

LexiconCode marked this pull request as ready for review March 30, 2024 10:45

LexiconCode merged commit 2146bab into main Apr 1, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use named loggers instead of setting root logger in natlinkcore library #72

Use named loggers instead of setting root logger in natlinkcore library #72

LexiconCode commented Mar 30, 2024 •

edited

Loading

LexiconCode commented Mar 30, 2024 •

edited

Loading

LexiconCode commented Mar 30, 2024 •

edited

Loading

drmfinlay commented Mar 30, 2024

	def run() -> None:
	default_logger=logging.getLogger()
	dh = OutputDebugStringHandler()
	sh=logging.StreamHandler(sys.stdout)
	for h in [sh,dh]:
	default_logger.addHandler(h)

	default_logger.setLevel(logging.DEBUG)

	logging.debug(f"{__file__} run()")
	try:
	# # TODO: remove this hack. As of October 2021, win32api does not load properly, except if
	# # the package pywin32_system32 is explictly put on new dll_directory white-list
	# pywin32_dir = os.path.join(sysconfig.get_path('platlib'), "pywin32_system32")
	# if os.path.isdir(pywin32_dir):
	# os.add_dll_directory(pywin32_dir)

	#create a temporary logging handler, so we can log the startup of DAP

	config = NatlinkConfig.from_first_found_file(config_locations())

	dap_started = config.dap_enabled and startDap(config)
	logger=logging.getLogger("natlink")
	logger.setLevel(logging.DEBUG)

	main = NatlinkMain(logger, config)
	main.setup_logger()
	main.dap_started=dap_started
	for h in [sh,dh]:
	default_logger.removeHandler(h)

Use named loggers instead of setting root logger in natlinkcore library #72

Use named loggers instead of setting root logger in natlinkcore library #72

Conversation

LexiconCode commented Mar 30, 2024 • edited Loading

LexiconCode commented Mar 30, 2024 • edited Loading

LexiconCode commented Mar 30, 2024 • edited Loading

drmfinlay commented Mar 30, 2024

LexiconCode commented Mar 30, 2024 •

edited

Loading

LexiconCode commented Mar 30, 2024 •

edited

Loading

LexiconCode commented Mar 30, 2024 •

edited

Loading